Size FastAGI listen backlog via kernel cap instead of sysctl#65
Conversation
get_somaxconn() shelled out to `sysctl` and parsed OS-specific output to read the system somaxconn, branched on Linux vs Darwin, and raised NotImplementedError on every other platform -- so FastAGIServer could not even start on, e.g., Windows or BSD. Reading the value is unnecessary. listen() already caps the backlog to the live system maximum, so passing the largest value the call accepts yields exactly that maximum and still tracks an administrator's tuned-up somaxconn automatically. Set request_queue_size to INT_MAX (2**31 - 1) and let the kernel clamp it. INT_MAX rather than a fixed ceiling like 65535: on modern kernels somaxconn is a 32-bit value that can be tuned above 65535, so any fixed ceiling could undershoot; the kernel's own limit is the only safe cap. Verified that listen() clamps (not rejects) a large backlog on Linux and on macOS/BSD. Removes the platform/socket/subprocess imports (only get_somaxconn used them) and the Linux/macOS-only restriction from the README and AGENTS docs. The _LISTEN_BACKLOG constant carries the full rationale inline. Add tests: the server sets request_queue_size to INT_MAX (binding on an ephemeral port also proves listen() accepts it), and the sysctl helper is gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…review) Review panel convergence (all comment/test polish, no behavior change): - Extend the _LISTEN_BACKLOG comment with two facts the edge-case lane verified: the value must be exactly INT_MAX because CPython raises OverflowError for a listen() backlog above a signed 32-bit int (so a future "round up" would crash startup), and Windows accepts INT_MAX not via a somaxconn clamp but because Winsock's SOMAXCONN constant is itself 0x7fffffff, a sentinel meaning "use a maximum reasonable backlog". - Test: import and assert against _LISTEN_BACKLOG instead of re-hardcoding the literal, and assert it exceeds 65535 so the "no fixed ceiling" rationale is executable. - Test: also assert the module no longer imports subprocess/platform, a behavioral guard against reintroducing the shell-out that survives a rename of the old helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
itg-karthicr
left a comment
itg-karthicr
left a comment
There was a problem hiding this comment.
Review panel for PR #65.
critical
None.
warning
tests/test_fastagi_server.py:17: The regression test only checks_LISTEN_BACKLOG > 65535. A future fixed ceiling such as1000000passes this test and still breaks the documentedINT_MAXcontract frompystrix/agi/fastagi.py:65. Suggested fix: assert_LISTEN_BACKLOG == 2**31 - 1.
info
pystrix/agi/fastagi.py:74,README.md:83,CHANGELOG.md:13,AGENTS.md:70: The wording says the kernel caps to livesomaxconnin every case, butpystrix/agi/fastagi.py:69says Windows uses the WinsockINT_MAXsentinel instead. Suggested fix: say Unix-like platforms clamp to livesomaxconn, and Windows uses the Winsock sentinel.
verification
pytest -q: 55 passed, outside sandbox.pytest -q tests/test_fastagi_server.py: 2 passed, outside sandbox.ruff check .: passed.ruff format --check .: passed.- Local socket probe on Darwin 25.5.0:
listen(2**31 - 1)accepted, andlisten(2**31)raisedOverflowError.
| assert server.request_queue_size == _LISTEN_BACKLOG | ||
| # A fixed ceiling like 65535 could undershoot a tuned-up somaxconn; the | ||
| # backlog must exceed it so the kernel's own limit is the only cap. | ||
| assert _LISTEN_BACKLOG > 65535 |
There was a problem hiding this comment.
The regression needs to lock the exact contract here. _LISTEN_BACKLOG > 65535 still passes for a fixed ceiling like 1000000, but the production comment says the value must be exactly INT_MAX. Suggested fix: assert _LISTEN_BACKLOG == 2**31 - 1.
| # Winsock treats that exact value as a sentinel meaning "use a maximum | ||
| # reasonable backlog". So INT_MAX is the right value on every platform, though | ||
| # Windows honors it as that sentinel rather than as a tuned registry limit. | ||
| _LISTEN_BACKLOG = 2**31 - 1 # INT_MAX; the kernel caps this to the live somaxconn |
There was a problem hiding this comment.
This inline comment says the kernel caps this to live somaxconn for every platform. Lines 69-73 say Windows uses the Winsock INT_MAX sentinel instead. Suggested fix: split the wording. Unix-like platforms clamp to live somaxconn, and Windows uses the Winsock sentinel.
…review) Review feedback: - The regression test asserted only `_LISTEN_BACKLOG > 65535`, which a future fixed ceiling like 1000000 would pass while violating the documented exactly-INT_MAX contract. Assert `_LISTEN_BACKLOG == 2**31 - 1` instead. - The "kernel caps to live somaxconn" wording read as universal but only holds on Unix-like systems; Windows accepts INT_MAX via the Winsock SOMAXCONN sentinel, not a somaxconn clamp. Scope the wording accordingly in the inline comment, README, CHANGELOG, and AGENTS.md so it no longer contradicts the block comment's Windows note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both addressed in Warning — test locks only Info — clamp wording reads as universal. Fixed. The inline comment, README, CHANGELOG, and AGENTS now scope the Full suite: 55 passing, ruff clean. |
|
Update: review feedback is resolved as of
CI is green on all 8 checks (Lint, Package build, Documentation build, Python 3.9 through 3.13). Full suite: 55 passing, ruff clean. No open review threads remain. |
1 participant
Closes #32.
Background
_ThreadedTCPServer.get_somaxconn()shelled out tosysctlto read the systemsomaxconn, parsed OS-specific output (=on Linux,:on macOS), branched onplatform.system(), and raisedNotImplementedErroron any host that was neither Linux nor macOS. SoFastAGIServercould not start on Windows, BSD, or anything else — the "Linux and macOS only" caveat in the docs was a real functional restriction, not just documentation.The issue thread debated
socket.SOMAXCONN(a small compile-time constant, 128 historically, 4096 since Linux 5.4) versus reading the live runtime value. The maintainer's requirement was legitimate: the server must pick up an administrator's tuned-upnet.core.somaxconn, which the constant does not reflect.Approach
Reading the value is unnecessary.
listen()already caps the backlog to the live system maximum (net.core.somaxconnon Linux,kern.ipc.somaxconnon macOS/BSD), so passing a value at or above that maximum yields exactly the maximum. Pass the largest value the call accepts and let the kernel clamp:This is functionally identical to the old
sysctlread on every configuration (both end up atmin(what_we_pass, live_somaxconn)), but:somaxconnautomatically, with no per-start lookup.NotImplementedErrorcrash and the docs caveat).sysctlsubprocess and theplatform/socket/subprocessimports.Why
INT_MAXand not65535: on modern kernelssomaxconnis a 32-bit value (max 2147483647) that an administrator can tune above 65535, so any fixed ceiling could undershoot a tuned host.INT_MAXcannot — the kernel's own limit is then the only cap. The full rationale lives in the_LISTEN_BACKLOGcomment.Verified:
listen()clamps (does not reject) a large backlog on Linux (net.core.somaxconn) and on macOS/BSD (kern.ipc.somaxconn, per the FreeBSDlisten(2)semantics and regression test). The new binding test exercises this on the CI platform.Tests
New
tests/test_fastagi_server.py:request_queue_sizetoINT_MAX; binding on an ephemeral port also proveslisten()accepts the value rather than rejecting it.sysctl-spawning helper is gone (regression guard).Full suite: 55 passing, ruff clean.
Docs
README, AGENTS.md, and CHANGELOG updated to describe the kernel-cap approach and remove the Linux/macOS-only restriction.